Skip to content

fix: Pinecone define pod, pod_type#6357

Merged
anakin87 merged 10 commits into
deepset-ai:v1.xfrom
x110:feature/pinecone-document-store-config-options
Dec 4, 2023
Merged

fix: Pinecone define pod, pod_type#6357
anakin87 merged 10 commits into
deepset-ai:v1.xfrom
x110:feature/pinecone-document-store-config-options

Conversation

@x110
Copy link
Copy Markdown
Contributor

@x110 x110 commented Nov 20, 2023

Related Issues

Proposed Changes:

  • Added support for the pods and pod_type parameters in the initialization of PineconeDocumentStore.

How did you test it?

  • I've written a test using Haystack APIs to create a Pinecone index. The test involves querying the index properties through the Pinecone API and then verifying that the retrieved values align with the predefined configuration.
  • In Pinecone starter plan, all create_index calls ignore the pod_type parameter and limits pods to 1. So I was not able to conduct the test.

Notes for the Reviewer

  • To evaluate these changes, you can execute the test with a Pinecone subscription.
  • The index_config parameter has been excluded as it is deprecated in the Pinecone library.

Checklist

@x110 x110 requested a review from a team as a code owner November 20, 2023 12:11
@x110 x110 requested review from anakin87 and removed request for a team November 20, 2023 12:11
@x110 x110 requested a review from a team as a code owner November 20, 2023 12:31
@x110 x110 requested review from dfokina and removed request for a team November 20, 2023 12:31
@anakin87
Copy link
Copy Markdown
Member

anakin87 commented Nov 21, 2023

Hello, @x110 and thanks for this contribution.

Some suggestions:

  • the new tests should be added to the existing Pinecone test file
  • some tests are failing. Please look at the errors and try to fix them. If you need help, feel free to ask...

@coveralls
Copy link
Copy Markdown
Collaborator

coveralls commented Nov 22, 2023 •
edited
Loading

Pull Request Test Coverage Report for Build 6977366512

Warning: This coverage report may be inaccurate.

We've detected an issue with your CI configuration that might affect the accuracy of this pull request's coverage report.
To ensure accuracy in future PRs, please see these guidelines.
A quick fix for this PR: rebase it; your next report should be accurate.

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 896 unchanged lines in 18 files lost coverage.
  • Overall coverage decreased (-0.2%) to 40.113%
Files with Coverage Reduction New Missed Lines %
nodes/file_converter/init.py 1 88.89%
utils/context_matching.py 1 95.7%
preview/components/builders/init.py 2 0.0%
preview/components/generators/init.py 2 0.0%
preview/components/routers/init.py 2 0.0%
nodes/preprocessor/base.py 4 81.82%
preview/components/builders/prompt_builder.py 12 0.0%
preview/document_stores/protocols.py 15 0.0%
preview/components/audio/whisper_remote.py 24 0.0%
preview/utils/filters.py 25 0.0%
Totals Coverage Status
Change from base Build 6929759681: -0.2%
Covered Lines: 10689
Relevant Lines: 26647
💛 - Coveralls

@anakin87 anakin87 self-assigned this Nov 22, 2023
@anakin87
Copy link
Copy Markdown
Member

anakin87 commented Nov 23, 2023

Could you please add an integration test similar to that deleted in eb72798?

For the API key, let's do something similar to

haystack/test/document_stores/test_pinecone.py

Line 43 in b34c35d

api_key=os.environ.get("PINECONE_API_KEY") or "fake-pinecone-test-key",

@masci masci changed the base branch from main to v1.x November 24, 2023 12:07
@masci masci added the 1.x label Nov 24, 2023
@x110
Copy link
Copy Markdown
Contributor Author

x110 commented Nov 25, 2023

Hi @anakin87 , Thank you for your feedback. I have added an integration test. let me know if everything looks good now.

@anakin87
Copy link
Copy Markdown
Member

anakin87 commented Nov 27, 2023

Hey, @x110!

I am sorry to have kept you waiting. I will be testing your PR in depth soon...
Please bear with me.

anakin87
anakin87 approved these changes Dec 4, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@anakin87 anakin87 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I finally spent some time on this PR and made some changes.

  • I set default values for pod and pod_type (otherwise Pinecone API raises errors)

  • I manually tested your changes with Starter and Standard plans. They work well!

  • I removed the integration test, since I discovered in our CI we run tests using the Starter Plan

Thanks for your contribution 💙

(Don't miss Haystack 2.0 Beta)

@anakin87 anakin87 merged commit f90694c into deepset-ai:v1.x Dec 4, 2023
@anakin87 anakin87 mentioned this pull request Dec 4, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@anakin87 anakin87 anakin87 approved these changes

@dfokina dfokina Awaiting requested review from dfokina dfokina is a code owner automatically assigned from deepset-ai/documentation

Assignees

@anakin87 anakin87

Labels

topic:document_store topic:pinecone topic:tests type:documentation Improvements on the docs

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Pinecone define pod, pod_type and index_config

4 participants

@x110 @anakin87 @coveralls @masci